Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature: branch protection conversation resolution #904

Merged
merged 3 commits into from
Dec 3, 2021
Merged

Feature: branch protection conversation resolution #904

merged 3 commits into from
Dec 3, 2021

Conversation

josh-barker
Copy link
Contributor

This PR addresses #868 and updates the following resources:

  • github_branch_protection
  • github_branch_protection_v3

You may find it easier to review commit by commit.

@bleggett
Copy link

I would like this - how difficult is the conflict resolution? Can I help move this along?

@josh-barker josh-barker force-pushed the feat/branch-protection-conversation-resolution branch from 93e41a0 to 2cc591a Compare October 20, 2021 21:05
@josh-barker
Copy link
Contributor Author

Hi @bleggett, thanks for the prompt. I've just rebased the code.

@josh-barker
Copy link
Contributor Author

Hi @jcudit, just wondering what the process is around getting a PR reviewed?

Thanks

@jcudit jcudit added this to the v4.19.0 milestone Nov 2, 2021
jcudit
jcudit reviewed Nov 2, 2021
View reviewed changes
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking like a promising contribution so far with the following caveats:

  • Can we get some clarity on the v4 dependency update?
  • Can we drop the edits to the _v3 resource? It has been in a frozen state since released so that we do not diverge from the recommended resource.

go.mod
@@ -10,7 +10,7 @@ require (
github.com/hashicorp/hcl/v2 v2.3.0 // indirect
github.com/hashicorp/terraform-config-inspect v0.0.0-20191212124732-c6ae6269b9d7 // indirect
github.com/hashicorp/terraform-plugin-sdk v1.7.0
github.com/shurcooL/githubv4 v0.0.0-20201206200315-234843c633fa
github.com/shurcooL/githubv4 v0.0.0-20210725200734-83ba7b4c9228
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get some insight into why this dependency is changing? Not a blocker, but hoping to understand what we're currently missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcudit The structs/variables for Conversation Resolution do not exist with the older version of githubv4.

@josh-barker
Copy link
Contributor Author

@jcudit There are existing issues relating to performance of the branch protection resource using GraphQL.
For example #606.

Therefore, I've implemented the configuration in both resources so that people have the option of choosing the more performant resource.

Personally, I can't use the github_branch_protection as I am managing too many repositories.

How did you want to proceed?

@jcudit
Copy link
Contributor

jcudit commented Nov 15, 2021

I think we can ship to both resources in this case, but overall discourage adding more to the github_branch_protection_v3 resource. Aiming to send this out in an upcoming release.

jcudit
jcudit approved these changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests well, thanks for the contribution!

@jcudit jcudit merged commit 502f594 into integrations:main Dec 3, 2021
@josh-barker josh-barker deleted the feat/branch-protection-conversation-resolution branch December 3, 2021 22:51
@mambax
Copy link

mambax commented Dec 15, 2021

Sorry but quick question. The docs state that this new require_conversation_resolution is optional.

Yet, after updating the provider our plans break:

github_team_repository.qa-microservice-streaming-api-omega-admin: Refreshing state... [id=9733:qa-microservice-streaming-api]

Error: Field 'requiresConversationResolution' doesn't exist on type 'BranchProtectionRule'

  with github_branch_protection.actions,
  on 30_actions.tf line 19, in resource "github_branch_protection" "actions":
  19: resource "github_branch_protection" "actions" {

Is that an issue or wanted behaviour?

@mambax mambax mentioned this pull request Dec 15, 2021
Closed
@nantiferov nantiferov mentioned this pull request Jan 20, 2022
Closed
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
@josh-barker
Feature: branch protection conversation resolution (integrations#904)
d76333c 
* feat: add conversation resolution variable on branch protection v3

* chore: update githubv4 package

* feat: add conversation resolution variable on branch protection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpereyrap fpereyrap fpereyrap approved these changes

@jcudit jcudit jcudit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v4.19.0
Development

Successfully merging this pull request may close these issues.
5 participants
@josh-barker @bleggett @jcudit @mambax @fpereyrap